-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added stats charts in xy-chart #75
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor comments mostly based on changes I've been making recently. This looks fantastic overall, thanks for adding it! 💯 🎉
@@ -15,6 +15,8 @@ import { | |||
LineSeries, | |||
PointSeries, | |||
StackedBarSeries, | |||
BoxPlotSeries, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this file is getting so big I think it'd be good to start moving the examples into their own files. I did this with <StackedAreaSeries />
, could we do that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good! I moved them out as a separate file
description: 'Box Plot Example', | ||
example: () => { | ||
const boxPlotData = statsData.map(s => s.boxPlot); | ||
const values = boxPlotData.reduce((r, e) => r.push(e.min, e.max, ...e.outliers) && r, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice concise way to write this 👌
<YAxis numTicks={4} /> | ||
<BoxPlotSeries | ||
data={boxPlotData} | ||
label="Test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the label
prop earlier this week! unnecessary now :)
/> | ||
<BoxPlotSeries | ||
data={boxPlotData} | ||
label="Test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto label
<YAxis numTicks={4} /> | ||
<ViolinPlotSeries | ||
data={violinData} | ||
label="Test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto label
@@ -0,0 +1,88 @@ | |||
import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import { Group } from '@vx/group'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto absolute imports
const offsetValue = horizontal ? y : x; | ||
const valueScale = horizontal ? xScale : yScale; | ||
const boxWidth = offsetScale.bandwidth(); | ||
const actualyWidth = Math.min(MAX_BOX_WIDTH, boxWidth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit actualWidth
?
|
||
const propTypes = { | ||
data: violinPlotSeriesDataShape.isRequired, | ||
label: PropTypes.string.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could remove this
|
||
const defaultProps = { | ||
boxWidth: null, | ||
stroke: '#000000', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use colors.darkGray
or colors.black
which aren't quiiiite black. most designers have told me to never use pure black ... some dark gray instead is more visually appealing.
const offsetValue = horizontal ? y : x; | ||
const valueScale = horizontal ? xScale : yScale; | ||
const boxWidth = offsetScale.bandwidth(); | ||
const actualyWidth = Math.min(MAX_BOX_WIDTH, boxWidth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit -- actualWidth
?
|
||
import { boxPlotSeriesDataShape } from '../utils/propShapes'; | ||
|
||
const propTypes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for renderTooltip
you'll have to add onMouseMove
, onMouseLeave
, and onClick
handlers here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! done
c979ae1
to
6fbc442
Compare
1 similar comment
dope! thanks for adding this 💯 should be able to cut a release later today |
This PR
<ViolinPlotSeries/>
and example<BoxPlotSeries/>
and exampleTODO